fix(core/singletask): guard BASE_MODEL_URL assignment against None in…#575
fix(core/singletask): guard BASE_MODEL_URL assignment against None in…#575g-k-s-03 wants to merge 2 commits into
Conversation
…itial_model_url, Issue 574 Signed-off-by: g-k-s-03 <govindsingh97704@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: g-k-s-03 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubeedge-bot
added
the
size/XS
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the _train method in singletask_learning.py to conditionally set or remove the BASE_MODEL_URL environment variable depending on whether initial_model is provided. The review feedback suggests a more robust check (if initial_model:) to handle both None and empty string values, preventing potential issues in downstream components that check for the presence of this environment variable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if initial_model is not None: | ||
| os.environ["BASE_MODEL_URL"] = initial_model | ||
| else: | ||
| os.environ.pop("BASE_MODEL_URL", None) |
There was a problem hiding this comment.
In Algorithm, self.initial_model_url is initialized as an empty string ("") by default. If initial_model_url is omitted from the configuration, initial_model can be passed as "" rather than None.
Setting os.environ["BASE_MODEL_URL"] = "" can cause issues in downstream components or subprocesses that check for the presence of the environment variable (e.g., "BASE_MODEL_URL" in os.environ).
To make this guard more robust against both None and empty strings, consider checking if initial_model: instead of if initial_model is not None:.
| if initial_model is not None: | |
| os.environ["BASE_MODEL_URL"] = initial_model | |
| else: | |
| os.environ.pop("BASE_MODEL_URL", None) | |
| if initial_model: | |
| os.environ["BASE_MODEL_URL"] = initial_model | |
| else: | |
| os.environ.pop("BASE_MODEL_URL", None) |
There was a problem hiding this comment.
Valid point; updated to if initial_model: to handle both None and empty string cases, since self.initial_model_url defaults to "" in the base class
…pty string Signed-off-by: g-k-s-03 <govindsingh97704@gmail.com>
2 participants
Description
SingleTaskLearning._train()unconditionally assignsos.environ["BASE_MODEL_URL"] = initial_model, whereinitial_modelis
Nonewheninitial_model_urlis absent from the algorithm YAML.This raises
TypeError: str expected, not NoneTypeimmediately.A secondary issue exists in multi-test-case benchmarks: when test
cases run sequentially in the same process via
testcase.py, a staleBASE_MODEL_URLset by a prior test case persists into subsequentones that omit
initial_model_url, causing the wrong pretrainedcheckpoint to be loaded silently.
Fix
Replaced the unconditional assignment with a guarded block:
The
popwith a default value clears any stale entry from a priortest case, preventing cross-test-case environment pollution.
Files Changed
core/testcasecontroller/algorithm/paradigm/singletask_learning/singletask_learning.pyFixes #574